PR Review: convolution: validate kernel and agg inputs (#3623)

Reviewed the full convolution.py and test_convolution.py on the PR branch, plus _validate_raster/_validate_boundary in utils.py. Ran the test file against the branch: 15 passed. The change does what it says, and the tests cover the cases that mattered.

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

• _validate_kernel (xrspatial/convolution.py:356) checks shape and dimensionality but not dtype. A 2D object-dtype or string kernel with odd sides passes the guard and still reaches numba, producing the cryptic TypingError this PR is meant to prevent. custom_kernel has the same gap, so this is not a regression, and dtype validation may be out of scope. Worth a one-line note in the PR if you are leaving it deliberately.

Nits (optional improvements)

• The duck-typed not hasattr(kernel, 'ndim') branch (convolution.py:356) reports only the type name, while the other two branches report the shape. That is the right call when there is no shape to report; just flagging that the three messages read a bit differently by design.

What looks good

• Keeping _validate_kernel separate from custom_kernel rather than reusing it is the right call: custom_kernel hard-requires np.ndarray, which would reject valid cupy kernels. Duck-typing on ndim/shape keeps all four backends working, and the odd-side-length contract matches custom_kernel.
• Kernel validation runs before the ArrayTypeFunctionMapping dispatch (convolution.py:514), so every backend gets the same up-front check through one code path.
• The _validate_raster guard added to convolution_2d (convolution.py:647) is load-bearing, not redundant with the re-validation inside convolve_2d. A numpy array has a .data attribute (a memoryview), so agg.data would otherwise succeed and fail later with 'memoryview' object has no attribute 'astype'. The guard turns that into a clear TypeError naming the parameter, and the test pins the old failure mode.
• Tests cover the paths that regressed: bad kernel types (None, 1D, 3D, list), even side lengths, non-DataArray input to convolution_2d, and the positive path.

Checklist

• Algorithm matches reference/paper (validation-only change, no algorithm touched)
• All implemented backends produce consistent results (validation is pre-dispatch, backend-agnostic)
• NaN handling is correct (unchanged)
• Edge cases are covered by tests
• Dask chunk boundaries handled correctly (unchanged)
• No premature materialization or unnecessary copies
• Benchmark exists or is not needed (validation cost is negligible)
• README feature matrix updated (not applicable; no new public function)
• Docstrings present and accurate

PR Review: convolution: validate kernel and agg inputs (follow-up)

Re-review after resolving the merge conflict with main. The branch was CONFLICTING; it is now MERGEABLE at 62ed3c54.

Merge conflict resolution

The only conflict was in xrspatial/tests/test_convolution.py, in the import block. This branch adds convolution_2d to the from xrspatial.convolution import ... line; main (via #3618/#3622) added a from xrspatial.tests.general_checks import cuda_and_cupy_available import for its new dask/cupy dtype tests. Both symbols are used, so I kept both. isort collapses the merged line to a single import (91 chars, under the repo's line_length=100), which matches main's style; isort --check-only and flake8 both pass on the merged file.

The branch also fast-forwarded past a stale local head to fork's tip before merging, so no prior commits were dropped. convolution.py merged cleanly with no conflict.

Validation logic (unchanged by the merge, re-confirmed)

• _validate_kernel duck-types on ndim/shape, rejects non-arrays, non-2D, and even side lengths before dispatch. Python lists and None hit the first branch and raise a ValueError naming kernel. This runs on the host before backend dispatch, so it covers numpy, cupy, and dask uniformly.
• convolution_2d now calls _validate_raster(agg, ndim=2) up front, which raises TypeError naming DataArray for a plain ndarray input, replacing the old 'memoryview' object has no attribute 'astype' failure.
• Kernels produced internally (circle_kernel, custom_kernel, focal callers) are all odd-sided, so the new guard does not reject any existing internal path.

Test result

pytest xrspatial/tests/test_convolution.py -> 22 passed. pytest xrspatial/tests/test_focal.py -> 351 passed (checks the internal convolve_2d callers are unaffected).

CI note

Merging main pulled in #3639's libjxl=0.11.* pin, so the branch carries the cog-validator fix and is current with main.

What looks good

• Error messages name the offending parameter and the actual shape/type, which is what the sweep was after.
• Even-kernel rejection now matches custom_kernel's existing odd-shape contract, closing the silent off-center-output path.

Checklist

• Merge conflict resolved, both imports preserved, isort/flake8 clean
• Validation raises clear ValueError/TypeError up front
• Internal callers (focal) unaffected
• Tests pass (convolution 22, focal 351)
• Branch current with main (libjxl pin included)